Skip to content

Stop following redirects, so the key never goes to the host they name - #36

Merged
dmccoystephenson merged 1 commit into
mainfrom
trace/do-not-follow-redirects
Nov 10, 2026
Merged

Stop following redirects, so the key never goes to the host they name#36
dmccoystephenson merged 1 commit into
mainfrom
trace/do-not-follow-redirects

Conversation

@dmccoystephenson

@dmccoystephenson dmccoystephenson commented Nov 8, 2026

Copy link
Copy Markdown
Member

Summary

  • _send now opens each report through a module-level opener (_OPENER) whose HTTPRedirectHandler declines every redirect, in place of urllib.request.urlopen. A 301/302/303 no longer re-sends Authorization: Bearer <key> to the Location host as an empty GET; every 3xx reaches the existing non-201 DEBUG line with its own status (answered 302) and the report is dropped.
  • install_opener was deliberately not used: it would change urlopen for the whole host program.
  • README "The wire format" gains a paragraph saying redirects are not followed, which matches the trace-client-java README's "Redirects are not followed" quoted in A 301, 302 or 303 answer sends the reporting key to the redirect target as an empty GET #31.

Closes #31

Test plan

  • New test_a_redirect_is_logged_and_dropped_never_followed_with_the_key covers 301, 302, 303, 307 and 308. The origin redirects to a second loopback server reached as localhost on another port. The test asserts that the target receives nothing and that the log names the redirect status at DEBUG.
  • Regression evidence (stash-and-run on Python 3.8.10): with the trace_client.py change stashed, the new test fails because GET /elsewhere reached the other host with the key. With the change restored, it passes.
  • test_an_error_answer_whose_body_cannot_be_read_still_logs_its_status now patches _OPENER.open instead of urllib.request.urlopen, because urlopen is no longer the call path. Its assertions are unchanged.
  • python3 -m unittest (Python 3.8.10): Ran 73 tests, OK.
  • CI matrix 3.8 / 3.10 / 3.12. Python 3.11 added 308 handling to HTTPRedirectHandler, so the 3.12 job is the check for that row.

Java parity

This brings Python in line with trace-client-java, which by its README already does not follow redirects. No Java change is needed. The Java README could not be re-read from this session's sandbox, so the parity claim relies on the quotation verified when #31 was filed.

Merge gate

This PR changes _send's behaviour on the wire, which is on this loop's do-not-auto-merge list (wire format must stay in lockstep with trace-client-java and the trace server). It is left open for owner review rather than merged autonomously, as #31 itself requested.

Deferred

This PR description was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

🤖 Generated with Claude Code

drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson @claude
Stop following redirects, so the key never goes to the host they name
c911068 
urlopen follows 301/302/303 by re-sending the Authorization header to the
Location target as an empty GET. Reports now go through an opener whose
redirect handler declines every redirect, so the 3xx reaches the existing
non-201 DEBUG line with its own status, as trace-client-java does.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dmccoystephenson

dmccoystephenson commented Nov 8, 2026

Copy link
Copy Markdown
Member Author

Self-review rubric (scored against the diff and command output; one intrinsic pass):

  • Scope: PASS. Three files changed, all for A 301, 302 or 303 answer sends the reporting key to the redirect target as an empty GET #31: trace_client.py (handler and opener), the test file (new test, handler gains Location/GET, one mock retargeted), and README.md (wire-format paragraph). 47 insertions, 2 deletions.
  • Tests-new: PASS. No public name was added. _RedirectsNotFollowed/_OPENER are private and are exercised end-to-end by test_a_redirect_is_logged_and_dropped_never_followed_with_the_key.
  • Tests-fix: PASS. Confirmed by stash-and-run on Python 3.8.10: with trace_client.py stashed, the new test FAILED (GET /elsewhere reached the other host carrying the key, as described in A 301, 302 or 303 answer sends the reporting key to the redirect target as an empty GET #31), and after restoring it the test PASSED.
  • Sibling structure: PASS. No new files. The test follows the loop-over-statuses shape of test_a_success_status_other_than_201_is_logged_as_not_delivered.
  • Sibling renames: PASS. Nothing was renamed.
  • Docs: PASS. README "The wire format" now states the redirect behaviour. The class docstring's "down, slow, or rejecting" promise still holds, because a redirect is one more dropped report.
  • Issue resolution: PASS. Both parts of the suggested fix in A 301, 302 or 303 answer sends the reporting key to the redirect target as an empty GET #31 are done: a non-following opener, and the redirect paragraph from the Java README.
  • CI: PASS. test (3.8), test (3.10) and test (3.12) are green on the head. 3.12 covers the post-3.11 308 handling.
  • Stdlib-only: PASS. The only added import is the package's own module in the tests. pyproject.toml is untouched.
  • Py3.8 floor: PASS. test (3.8) is green, and the change was also run locally on 3.8.10.
  • Never-raises / never-blocks: PASS. _OPENER.open sits in the same try as the urlopen it replaces. A declined redirect surfaces as HTTPError, which the existing branch handles. report() is unchanged.
  • Log level: PASS. No _LOG call was added. The existing DEBUG line reports answered 3xx.
  • Version agreement: PASS. No version string changed.
  • Exports: PASS. No public names were added.
  • Java parity noted: PASS. The PR body states that no Java change is needed and that the quotation from the Java README is taken from A 301, 302 or 303 answer sends the reporting key to the redirect target as an empty GET #31.

Judgment call for the owner:

  • trace_client/trace_client.py, at the _OPENER = urllib.request.build_opener(...) line: the opener is built at import time, so its ProxyHandler reads the proxy environment variables (HTTPS_PROXY etc.) at import. Before this change, urlopen built its cached opener on the first report. A program that sets proxy variables after importing the client but before its first report would no longer have them honoured. Building the opener lazily in _send (or per request) would restore the old timing if that case matters.

Merge gate: wire-format change (_send), which is on the do-not-auto-merge list. Left open for owner review.

This review comment was drafted during a Gardener session (https://github.com/Stephenson-Software/gardener).

drafted by Claude on behalf of Daniel Stephenson

@dmccoystephenson
dmccoystephenson merged commit 1613bed into main Nov 10, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

A 301, 302 or 303 answer sends the reporting key to the redirect target as an empty GET

1 participant

@dmccoystephenson